Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(Spanner): Inline Begin Transaction #6708

Merged
merged 34 commits into from
Jan 3, 2024
Merged

feat(Spanner): Inline Begin Transaction #6708

merged 34 commits into from
Jan 3, 2024

Conversation

ajupazhamayil
Copy link
Contributor

@ajupazhamayil ajupazhamayil commented Oct 13, 2023 •
edited by vishwarajanand
Loading

go/php-spanner-ilb

BREAKING_CHANGE_REASON=marked TransactionalReadInterface as internal, added a new function into it, changed public method rollback signature.

@product-auto-label product-auto-label bot added the api: spanner Issues related to the Spanner API. label Oct 13, 2023
@ajupazhamayil ajupazhamayil force-pushed the spanner-ilb branch 2 times, most recently from bf4c6bc to a3860ac Compare October 16, 2023 18:57
@ajupazhamayil
Copy link
Contributor Author

Resolving the integration tests for now. Once the design is approved, will work on the rest.

@ajupazhamayil ajupazhamayil force-pushed the spanner-ilb branch 3 times, most recently from 829e345 to 5f5a66c Compare October 17, 2023 13:21
bshaffer
bshaffer reviewed Oct 27, 2023
View reviewed changes
Copy link
Contributor

@bshaffer bshaffer left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some concerns with this PR. The major ones include:

  • Passing everything by reference - we do not want to do this.
  • $this->transactionId is type string and array. - we want it to be only one or the other
  • we add a new argument $transactionId, instead of using existing $options

More minor concerns:

  • Using and instead of && - never use and

This is just a first pass. Without diving deeper into the implementation, it's hard to advise further.

Spanner/src/Operation.php Outdated Show resolved Hide resolved
Spanner/src/Operation.php Outdated Show resolved Hide resolved
Spanner/src/Result.php Outdated Show resolved Hide resolved
Spanner/src/Result.php Outdated Show resolved Hide resolved
Spanner/src/Transaction.php Outdated Show resolved Hide resolved
Spanner/src/Transaction.php Outdated Show resolved Hide resolved
Spanner/src/Transaction.php Outdated Show resolved Hide resolved
Spanner/src/Transaction.php Outdated Show resolved Hide resolved
Spanner/src/Transaction.php Outdated Show resolved Hide resolved
Spanner/src/Transaction.php Outdated Show resolved Hide resolved
@ajupazhamayil
Copy link
Contributor Author

ajupazhamayil commented Oct 30, 2023 •
edited
Loading

I have a lot of concerns with this PR. The major ones include:

  • Passing everything by reference - we do not want to do this.

We will have to pass the object at least to make the communication between other classes possible.

  • $this->transactionId is type string and array. - we want it to be only one or the other

This we can achieve by adding a new attribute, will change this behaviour once the design is approved.

  • we add a new argument $transactionId, instead of using existing $options

This is a great idea! Anand also mentioned this. Added this as an alternative option in the design.

More minor concerns:

  • Using and instead of && - never use and

We will replace this.

This is just a first pass. Without diving deeper into the implementation, it's hard to advise further.

Thank you for the review, will discuss this further to have a deeper understanding.

@ajupazhamayil ajupazhamayil force-pushed the spanner-ilb branch 5 times, most recently from bd62965 to 4fb21a8 Compare October 31, 2023 14:49
vishwarajanand
vishwarajanand requested changes Nov 30, 2023
View reviewed changes
Copy link
Contributor

@vishwarajanand vishwarajanand left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls fix the nits and resolve pending comments. Design wise, this looks good. This required a lot of patience from your side, thank you. 🥇

There are several breaking changes, so will want to re-look at this before giving a final approval.

@ajupazhamayil
Copy link
Contributor Author

Hi @vishwarajanand, Thank you for the review, I have resolved all your comments, please have a look and let me know how this looks.

vishwarajanand
vishwarajanand approved these changes Dec 4, 2023
View reviewed changes
Copy link
Contributor

@vishwarajanand vishwarajanand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some nits, LGTM

@ajupazhamayil ajupazhamayil added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Dec 5, 2023
@ajupazhamayil
Copy link
Contributor Author

Following our team discussion, we've decided to target a January merge for this PR. A do not merge label has been added for now.

vishwarajanand
vishwarajanand approved these changes Jan 3, 2024
View reviewed changes
Copy link
Contributor

@vishwarajanand vishwarajanand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@vishwarajanand vishwarajanand removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 3, 2024
@ajupazhamayil ajupazhamayil enabled auto-merge (squash) January 3, 2024 09:23
@ajupazhamayil ajupazhamayil merged commit f6d8dee into main Jan 3, 2024
27 checks passed
@ajupazhamayil ajupazhamayil deleted the spanner-ilb branch January 3, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vishwarajanand vishwarajanand vishwarajanand approved these changes

@bshaffer bshaffer bshaffer approved these changes

Assignees

@ajupazhamayil ajupazhamayil

Labels
api: spanner Issues related to the Spanner API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ajupazhamayil @bshaffer @vishwarajanand